fix(net): Bound the transport checksum by the network header's length by daniel-noland · Pull Request #1778 · githedgehog/dataplane

daniel-noland · 2026-08-28T23:15:27Z

The defect

Packet::new kept everything after the parsed headers as the payload, and nothing ever trimmed the buffer to the IPv4 total length / IPv6 payload length (TrimFromEnd existed but was never called). do_serialize then handed that whole slice to the checksum code, so ethernet padding was summed as if it were L4 payload.

Ethernet pads frames out to 64 octets including the Ethernet header, so any IPv4 frame under that was affected:

protocol	pseudo header length source	effect of padding
TCP	`header_len + payload.len()`	always wrong, off by the pad length, even for zeroed padding
ICMPv6	`payload.len() + header_len`	same, but IPv6 frames are ≥ 62 octets so rarely padded
UDP	the UDP header's own `length` field	wrong only when the padding is non-zero
ICMPv4	no length in the checksum	wrong only when the padding is non-zero

The sharp edge is IPv4 TCP: a bare ACK, RST, FIN, or option-less SYN is 54 octets on the wire. Every one of those this dataplane forwarded left with a checksum the far end drops. Flows carrying TCP timestamps produce 66-octet frames and escaped.

coderabbitai · 2026-08-28T23:15:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5ccde373-fea6-46a3-b8ce-a3846e9234ff

📥 Commits

Reviewing files that changed from the base of the PR and between a2916ca and f41630d.

📒 Files selected for processing (2)

net/src/headers/mod.rs
net/src/packet/mod.rs

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

📝 Walkthrough

Walkthrough

Changes

Checksum payload bounds

Layer / File(s)	Summary
Transport payload length contract `net/src/headers/mod.rs`	Adds `Headers::transport_payload_len` for IPv4 and IPv6 payload bounds. Documents its use for checksum updates.
Checksum update integration and tests `net/src/packet/mod.rs`	Bounds checksum input to the declared transport payload when available. Preserves truncated-packet handling. Tests TCP, UDP, and ICMPv4 padding and payload cases.

Suggested reviewers: sergeymatov, fredi-raspall, qmonnet

Merge Risk: ⚪ Minimal · up to f4163

The PR bounds transport checksum calculations to the network packet length so Ethernet padding is not included. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: bounding transport checksum input by the network header length.
Description check	✅ Passed	The description directly explains the checksum defect, its cause, affected protocols, and the intended fix.
Docstring Coverage	✅ Passed	Docstring coverage is 94.12% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-08-28T23:21:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

An ethernet frame shorter than 60 octets arrives padded, and that padding sat inside the slice we summed. TCP and ICMPv6 take their pseudo header length from the slice, so a bare 54-octet ACK left with a checksum off by the pad length. This is wrong even when the padding is zeroed, which is why UDP and ICMPv4 usually survived it and TCP did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>

mvachhar

Do we want to have a fix for the serializer when there is a short payload or is that a separate PR?

mvachhar · 2026-08-29T00:00:50Z

Oh, and FYI, the commit message is just wrong. 802.3 includes the header in the frame and size, so the threshold is not a frame size of 60 bytes, but a frame size of 64 bytes. I didn't check its numbers on the IP packet size. Annoying. Fix if you want, I won't hold up the PR for it.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

daniel-noland force-pushed the pr/daniel-noland/padding-checksum branch from 0005119 to f41630d Compare August 28, 2026 23:41

daniel-noland self-assigned this Aug 28, 2026

daniel-noland added ci:+merge-ready Run all checks which will be run in the merge queue regardless of label status ci:+release Enable VLAB release tests bug Something isn't working labels Aug 28, 2026

mvachhar approved these changes Aug 28, 2026

View reviewed changes

daniel-noland marked this pull request as ready for review August 29, 2026 02:18

daniel-noland requested a review from a team as a code owner August 29, 2026 02:18

daniel-noland requested review from sergeymatov and a lite review from Copilot and removed request for a team August 29, 2026 02:18

daniel-noland added this pull request to the merge queue Aug 29, 2026

Copilot AI reviewed Aug 29, 2026

Merged via the queue into main with commit d85bdfa Aug 29, 2026
41 of 69 checks passed

daniel-noland deleted the pr/daniel-noland/padding-checksum branch August 29, 2026 02:57

Navigation Menu

Uh oh!

fix(net): Bound the transport checksum by the network header's length - #1778

fix(net): Bound the transport checksum by the network header's length#1778
daniel-noland merged 1 commit into
mainfrom
pr/daniel-noland/padding-checksum

daniel-noland commented Aug 28, 2026 •

edited by mvachhar

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

codecov Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

mvachhar left a comment

Uh oh!

mvachhar commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

daniel-noland commented Aug 28, 2026 • edited by mvachhar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The defect

Uh oh!

coderabbitai Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

codecov Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvachhar left a comment

Choose a reason for hiding this comment

Uh oh!

mvachhar commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daniel-noland commented Aug 28, 2026 •

edited by mvachhar

Loading

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

codecov Bot commented Aug 28, 2026 •

edited

Loading